Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENH: Update reorderItems() to use ORM where possible #336

Merged
merged 2 commits into from
Aug 3, 2022
Merged

ENH: Update reorderItems() to use ORM where possible #336

merged 2 commits into from
Aug 3, 2022

Conversation

chrispenny
Copy link
Contributor

@chrispenny chrispenny commented Mar 3, 2022 •
edited
Loading

Description

Closes #335

Based onto 3, but please feel free to update that with what you think is appropriate.

I'm not 100% familiar with all of the different Lists that we have, but I think it would only be the ManyManyList that we still need to process with SQL Queries, as their DB records have no representation in the ORM.

Additions

Just added a cheeky little extension point for onBeforeReorderItems, because "why not?".

@chrispenny chrispenny changed the base branch from 3.3 to 3 March 3, 2022 20:25
@michalkleiner
Copy link
Collaborator

I'm all for simplification, though it seems suspicious that such a big removal will go without an impact in tests. Does it mean something significantly changed somewhere else to still support the reordering?

@chrispenny
Copy link
Contributor Author

chrispenny commented Mar 3, 2022 •
edited
Loading

Hi @michalkleiner,

The module already supported sorting through the ORM, the only thing was that it only used the ORM for Versioned DataObjects. All I've done is change the criteria for when the module uses the ORM vs SQL Queries.

The relevant code for sorting through the ORM is still here:
https://github.com/symbiote/silverstripe-gridfieldextensions/pull/336/files#diff-ad5bbd1d14499e77c6ef3223c6c3746fa71333fec5831b4c3fcfa7a5e40f09a0R618

@chrispenny chrispenny force-pushed the enh/orderable-rows-orm branch from 07fd599 to ad59dad Compare July 7, 2022 20:02
@chrispenny
Copy link
Contributor Author

Hi @michalkleiner

We now have tests covering:

  • has_many un-Versioned
  • has_many subclass un-Versioned
  • has_many subclass Versioned
  • many_many un-Versioned
  • many_many Versioned
  • many_many with through model un-Versioned
  • many_many with through model Versioned

Please let me know if there is anything else I can do to increase confidence in this change.

@GuySartorelli
Copy link
Collaborator

I'm slowly reviewing this inbetween other work.
If someone else has more time and wants to review it, please go ahead - but otherwise I'll get there eventually.

GuySartorelli
GuySartorelli approved these changes Aug 1, 2022
View reviewed changes
Copy link
Collaborator

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, works correctly locally, and the new tests are fantastic. Thanks for doing this.
Can you please rebase and resolve the merge conflict?

@chrispenny chrispenny force-pushed the enh/orderable-rows-orm branch from 446618b to c2faf9e Compare August 3, 2022 19:17
@chrispenny chrispenny force-pushed the enh/orderable-rows-orm branch from c2faf9e to 5041e6c Compare August 3, 2022 19:19
@chrispenny
Copy link
Contributor Author

Thanks, @GuySartorelli! Rebase complete.

I'm not sure why, but running the workflow needs approval, if you could please 😃 .

@GuySartorelli GuySartorelli merged commit 23a5154 into symbiote:3 Aug 3, 2022
@chrispenny chrispenny deleted the enh/orderable-rows-orm branch August 3, 2022 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuySartorelli GuySartorelli GuySartorelli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Question: Could GridFieldOrderableRows::reorderItems() please use ORM for non-Versioned models?
3 participants
@chrispenny @michalkleiner @GuySartorelli